-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for UWG4 thermostats #200
Conversation
|
||
# WG4-only fields: | ||
temperature: int | None = None | ||
set_point_temperature: int | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to stick with a single, data-only Thermostat
class instead of introducing subclassing here. Especially since more models could be supported in the future, it seems better for code to branch on "is sensor_mode None?" than "is this thermostat a subclass of XXX?"
"VacationEndDay": _fmt(thermostat.vacation_end_time), | ||
"VacationTemperature": thermostat.vacation_temperature, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code previously in update_group_request
was moved here.
A JSON object containing the update. | ||
""" | ||
if temperature is not None: | ||
self.resource.temperatures[regulation_mode] = temperature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important! Previously, calling set_regulation_mode
would actually change the temperatures
variable on that thermostat.
This seemed funky to me for a couple reasons: first, this change takes effect even if the subsequent request fails due to auth failure, network unavailability, etc., and it is never rolled back in that case. Second, it feels like the Thermostat
object should be a forever-immutable snapshot of a thermostat's state at some point in time instead of a mutable object that changes.
So… I simply removed this from the refactored code.
@@ -31,7 +31,7 @@ python = "^3.9" | |||
yarl = ">=1.6.0" | |||
|
|||
[tool.poetry.group.dev.dependencies] | |||
aresponses = "^2.1.6" | |||
aresponses = "^3.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This silences a warning when running tests, not a huge deal.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 9 9
Lines 251 339 +88
Branches 36 51 +15
=========================================
+ Hits 251 339 +88 ☔ View full report in Codecov by Sentry. |
@robbinjanssen Here we go! This PR maintains 100% test coverage, hooray. It should be paired with a PR for the Home Assistant integration, which I'll work on next. |
min_temperature: int | ||
max_temperature: int | ||
temperatures: dict[int, int] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the temperatures
dict in favor of individual fields (comfort_temperature
, boost_temperature
, etc.).
This felt better as it's closer to the data on the wire and allows the type system to express which temperatures must be present (manual_temperature
, comfort_temperature
) and which are optional (vacation_temperature
is optional since WG4-thermostats don't support it).
|
||
# The OWD5 API requires computing the value: | ||
if self.regulation_mode == REGULATION_SCHEDULE and self.schedule: | ||
return Schedule.from_json(self.schedule).get_active_temperature() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the schedule is now lazily parsed in get_target_temperature
, and only if needed by the current regulation mode.
I'm open to changing it, but this option seemed simplest.
Awesome! I'll have a look (and will test) tonight! |
As a follow-up, I plan to support subscribing for updates via the push mechanism that the WG4 API offers instead of polling. But that will come in a separate PR. |
""" | ||
Fix corrupt date formats used by the WG4 API. | ||
|
||
Bizarrely the WG4 API sometimes--but not always--sends dates of the form: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work! I'll test it and see if everything is still working on my end :-)
This is a pretty monolithic PR but it closes #178 by fully supporting WG4-series thermostats.